fix double clearing on autoclear #2666
Conversation
| // but for now, ensure this happens in the right order by clearing data here too, if needed. | ||
| tabsModel = TabsModel(desktop: isPadDevice) | ||
| tabsModel.save() | ||
| previewsSource.removeAllPreviews() |
There was a problem hiding this comment.
Don't we need to still remove previews?
DuckDuckGo/MainViewController.swift
Outdated
| @@ -998,6 +982,7 @@ class MainViewController: UIViewController { | |||
| currentTab?.progressWorker.progressBar = nil | |||
| currentTab?.chromeDelegate = nil | |||
|
|
|||
| tab.delegate = self | |||
There was a problem hiding this comment.
I see what you mean by delaying the moment delegate is attached. The problem here is that before delegate is set, the view is apparently already loaded. The most likely place that could fail are the ones that use delegate methods which return a value like tabDidRequestSearchBarRect or tabCheckIfItsBeingCurrentlyPresented which could glitch the TabUI itself during its setup depending on the code flows (there could be other glitches too, but these are most likely).
We could investigate code paths to "fix" this now, but overall this is not a foolproof solution. I'm afraid that with TabManager being a Factory, a binding must occur as it was.
But looking at the code, I think it could remain as it was - just maybe don't pass self it through TabManager init as it's awkward for weak vars but explicitly invoke a manager.tabDelegate = self in MVC init.
DuckDuckGo/TabViewController.swift
Outdated
| weak var delegate: TabDelegate? | ||
| weak var delegate: TabDelegate? { | ||
| didSet { | ||
| assert(delegate != nil) |
There was a problem hiding this comment.
This is getting triggered when using fire button, as prepareForDataClearing sets delegate to nil
This reverts commit cebfbf5.
This reverts commit cebfbf5.
This reverts commit cebfbf5. # Conflicts: # DuckDuckGo/TabManager.swift
Task/Issue URL: https://app.asana.com/0/414709148257752/1206731143449260/f
Tech Design URL:
CC:
Description:
Steps to test this PR:
Test standard fire button and fire proofing usage
Autoclear with Terminate
Autoclear with Terminate and Delay
shouldClearDatafunctionAutoclear with tabs only